New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

DM-41025: Add SDSS_65mm filter options to grating slot for LATISS. #473

Merged

edennihy merged 1 commit into main from tickets/DM-41025

Oct 10, 2023

Contributor

edennihy commented Oct 5, 2023

No description provided.

edennihy requested a review from timj

October 5, 2023 22:01

Contributor

ktlim commented Oct 5, 2023

Might be nice to filter out combinations that will never be used; right now the nested loops will make every combination.

Contributor

ktlim commented Oct 5, 2023

Also, you have aliases for the filter in the filter wheel (new line 357); do you need aliases for the filter in the grating wheel?

edennihy force-pushed the tickets/DM-41025 branch 2 times, most recently from 3d657d5 to e165117 Compare

October 5, 2023 22:52

Contributor Author

edennihy commented Oct 5, 2023

Do you have a suggestion for a quick way to filter out all of the (anticipated) unused combinations?

Contributor Author

edennihy commented Oct 5, 2023 •

edited

Loading

To your second point, I noticed the SDSS65mm filter definitions above did not have alias definitions, so I wasn't sure if that was needed. Same thing for the afw_name, unless those are being defined elsewhere. Although I just realized that new_alias definition will fail anyways since it is expecting a string..let me update that as well.

Contributor Author

edennihy commented Oct 5, 2023

Okay, I added some simple logic to only allow the (empty~SDSS_65mm) combination and a check on the grating alias if needed.

edennihy changed the title ~~Add SDSS_65mm filter options to grating slot for LATISS.~~ DM-41025: Add SDSS_65mm filter options to grating slot for LATISS.

Member

timj commented Oct 6, 2023

How many combinations do you end up with after this change?

$ butler create tmp
$ butler register-instrument tmp lsst.obs.lsst.Latiss
$ butler query-dimension-records tmp physical_filter

?

Contributor Author

edennihy commented Oct 6, 2023 •

edited

Loading

The total number of filters prior to change is 701, total number after is 707. The are a lot of combinations with the pinhole masks that we could filter as well if we want. For example, order blocking filters will not be used with the pinhole masks.

timj requested changes

View reviewed changes

Member

timj left a comment

I think this needs a bit of tidying up.

It's great that you managed to filter out all the combinations. I agree that another pass in the future to remove impossible combinations would be great.

python/lsst/obs/lsst/filters.py Outdated

@@ @@ -325,6 +325,18 @@ def addFilter(filter_dict, band, physical_filter): @@
                                   "pinhole_2_4_0500",
                                   "pinhole_2_4_0200",
                                   "pinhole_2_4_0100",
+                                  FilterDefinition(physical_filter="SDSSu_65mm",

Member

timj Oct 10, 2023

Can you add a comment above this block explaining why these filters are nominally in the gratings list?

python/lsst/obs/lsst/filters.py Outdated

@@ @@ -325,6 +325,18 @@ def addFilter(filter_dict, band, physical_filter): @@
                                   "pinhole_2_4_0500",
                                   "pinhole_2_4_0200",
                                   "pinhole_2_4_0100",
+                                  FilterDefinition(physical_filter="SDSSu_65mm",
+                                                   band="u"),
+                                  FilterDefinition(physical_filter="SDSSg_65mm",

Member

timj Oct 10, 2023

I worry a bit that this duplication of class constructors with the filter list is going to leave us susceptible to typos in the future. Maybe we define a sdss_filters list and then use *sdss_filters in here and the filter list above? That would guarantee that we really are duplicating.

python/lsst/obs/lsst/filters.py Outdated

                       # FilterDefinition is a frozen dataclass
-                      new_name = FILTER_DELIMITER.join([filter.physical_filter, grating])
+                      if hasattr(grating, "physical_filter"):

Member

timj Oct 10, 2023

I think you can do something like:

grating_name = getattr(grating, "physical_filter", grating)
new_name = FILTER_DELIMITER.join([filter.physical_filter, grating_name])

python/lsst/obs/lsst/filters.py Outdated

+                      else:
+                          new_aliases = {FILTER_DELIMITER.join([a, grating]) for a in filter.alias}
+                      # For gratings set the band to the band of the filter. If an SDSS_65mm

Member

timj Oct 10, 2023

This commit is far more specific than the code that is written here. The comment should be talking about what happens if the grating is a filter definition. SDSS_65mm is never checked.

python/lsst/obs/lsst/filters.py

+                      # filter is installed in the grating slot, define the band as the band
+                      # of the SDSS_65mm filter.
+                      if hasattr(grating, "band"):
+                          combo = FilterDefinition(physical_filter=new_name,

Member

timj Oct 10, 2023

Since the above code is mainly working out the parameters for these calls, maybe use one isinstance instead to calculate the parameters instead of doing lots of hasattr tests?

timj approved these changes

View reviewed changes


          Add SDSS_65mm filter options to grating slot for LATISS.

f5a405a

edennihy force-pushed the tickets/DM-41025 branch from ebaca9c to f5a405a Compare

October 10, 2023 19:32

edennihy merged commit 32fff34 into main

3 checks passed

edennihy deleted the tickets/DM-41025 branch

October 10, 2023 19:55

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet